chore: improve webrtc negotiation timeouts #38191
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces the coarse 'accepted' client state with granular signaling states, tracks SDP send/receive progress, maps specific client-state timeouts to new hangup reasons (not-answered, timeout-remote-sdp, timeout-local-sdp, timeout-activation), and propagates negotiation finished state to the negotiation manager. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Call
participant Negotiation
participant Callee
Caller->>Call: accept() → enter 'accepted' (granular flow)
Call->>Call: start activation timeout
Callee->>Call: send offer (remote SDP)
Call->>Call: receivedRemoteSdp = true
Call->>Call: update client state -> 'generating-local-sdp' (or 'waiting-for-answer')
Call-->>Call: clear/adjust timeouts
Caller->>Negotiation: create local SDP / send answer
Negotiation->>Call: mark sentLocalSdp = true
Call->>Call: deliverSdp(), start activation timeout
Call->>Call: update client state -> 'activating'
Negotiation->>Negotiation: finish()
Negotiation->>Call: notify finished
Call->>Call: clear activation timeout, set state 'active'
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (8)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pierre-lehnen-rc
force-pushed
the
chore/media-calls-multi-timeout
branch
from
bf2d6c7 to
cf69554
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38191 +/- ##
===========================================
- Coverage 70.67% 70.67% -0.01%
===========================================
Files 3133 3133
Lines 108369 108373 +4
Branches 19488 19548 +60
===========================================
+ Hits 76593 76595 +2
+ Misses 29775 29772 -3
- Partials 2001 2006 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/16 14:28 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Fri, 16 Jan 2026 14:28:58 GMT |
pierre-lehnen-rc
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/media-signaling/src/definition/client.ts">
<violation number="1" location="packages/media-signaling/src/definition/client.ts:8">
P3: Typo in comment: "Thw" should be "The".</violation>
</file>
<file name="packages/media-signaling/src/lib/Call.ts">
<violation number="1" location="packages/media-signaling/src/lib/Call.ts:1043">
P3: The `'accepting'` state is missing from the switch statement. Timeouts are set for this state in `accept()`, but when they fire, this will return the generic `'timeout'` reason instead of a more descriptive one like `'timeout-acceptance'`. Consider adding a case for consistency with other states.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/media-signaling/src/lib/Call.ts (1)
1043-1057: The'accepting'state is missing from this switch.Timeouts are set for the
'accepting'state inaccept()(line 537), but this method will return the generic'timeout'reason for that state instead of a specific one like'timeout-acceptance'.This was flagged in a previous review. Consider adding a case for consistency with other states.
🧹 Nitpick comments (1)
packages/media-signaling/src/definition/signals/client/hangup.ts (1)
39-43: Minor ordering inconsistency with type definition.The new hangup reasons are placed after
'timeout'here (line 39), but inIClientMediaCall.tsthey appear before'timeout'. While this doesn't affect validation behavior, maintaining consistent ordering between the type definition and schema enum improves readability and reduces maintenance burden.♻️ Suggested reordering for consistency
reason: { type: 'string', enum: [ 'normal', 'remote', 'rejected', 'unavailable', 'transfer', - 'timeout', 'not-answered', 'timeout-remote-sdp', 'timeout-local-sdp', 'timeout-activation', + 'timeout', 'signaling-error',
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/meteor/server/services/media-call/service.tspackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/media-signaling/src/definition/client.tspackages/media-signaling/src/definition/signals/client/hangup.tspackages/media-signaling/src/definition/signals/client/local-state.tspackages/media-signaling/src/lib/Call.tspackages/media-signaling/src/lib/NegotiationManager.tspackages/media-signaling/src/lib/services/webrtc/Negotiation.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/services/media-call/service.tspackages/media-signaling/src/lib/NegotiationManager.tspackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/media-signaling/src/lib/services/webrtc/Negotiation.tspackages/media-signaling/src/lib/Call.tspackages/media-signaling/src/definition/client.tspackages/media-signaling/src/definition/signals/client/local-state.tspackages/media-signaling/src/definition/signals/client/hangup.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 36718
File: packages/media-signaling/src/lib/Call.ts:633-642
Timestamp: 2025-09-23T00:27:05.438Z
Learning: In PR `#36718`, pierre-lehnen-rc prefers to maintain consistency with the old architecture patterns for DTMF handling rather than implementing immediate validation improvements, deferring enhancements to future work.
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-12-18T15:18:31.688Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:31.688Z
Learning: In apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx, for internal call history items, the item.contactId is guaranteed to always match either the caller.id or callee.id in the call data, so the contact resolution in getContact will never result in undefined.
Applied to files:
apps/meteor/server/services/media-call/service.ts
🧬 Code graph analysis (1)
packages/media-signaling/src/lib/Call.ts (2)
packages/media-signaling/src/definition/client.ts (1)
ClientState(1-12)packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
CallHangupReason(30-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (12)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
36-40: LGTM! New timeout-specific hangup reasons align well with PR objectives.The granular hangup reasons (
not-answered,timeout-remote-sdp,timeout-local-sdp,timeout-activation) provide better diagnostics for WebRTC negotiation failures. Retaining the generictimeoutat line 40 ensures backward compatibility.packages/media-signaling/src/definition/client.ts (1)
5-8: LGTM! Granular client states enable precise timeout tracking.The new states (
waiting-for-offer,waiting-for-answer,generating-local-sdp,activating) correctly model the WebRTC negotiation lifecycle, enabling the timeout to be reset at each progression step as per the PR objectives.packages/media-signaling/src/lib/NegotiationManager.ts (3)
13-15: LGTM! Clean getter for checking negotiation completion state.This getter enables the Call layer to determine if any negotiation has successfully finished, which is essential for distinguishing between
timeout-activation(after negotiation) andtimeout-remote-sdp/timeout-local-sdp(during negotiation).
36-37: LGTM! Proper initialization and tracking of finished negotiations.The new
highestFinishedNegotiationIdproperty is correctly initialized tonulland updated when a negotiation finishes, providing the state needed for thehasFinishedAnyNegotiationgetter.Also applies to: 51-51
213-215: LGTM! Correctly tracks finished negotiation state.The conditional update ensures
highestFinishedNegotiationIdis only set when a negotiation completes successfully (negotiation.finished === true), not when it's merely ended or cancelled.packages/media-signaling/src/definition/signals/client/local-state.ts (1)
44-56: LGTM! Schema enum correctly reflects updated ClientState type.The expanded
clientStateenum properly includes the new granular states and removes'accepted', matching the type definition inclient.ts. Note thatcallState(line 39) still correctly includes'accepted'since that represents the high-level call lifecycle, distinct from the client-specific negotiation state.apps/meteor/server/services/media-call/service.ts (1)
231-237: LGTM! New hangup reason guards correctly map to call history states.The order is correct: checking
'not-answered'explicitly before thestartsWith('timeout')ensures proper state mapping. The prefix check elegantly handles all timeout variants (timeout-remote-sdp,timeout-local-sdp,timeout-activation, and generictimeout).packages/media-signaling/src/lib/services/webrtc/Negotiation.ts (1)
21-23: LGTM! Clean implementation of negotiation completion tracking.The
finishedflag correctly distinguishes successful completions from failures/skips:
- Only set when
_startedProcessing && !_failedguards passend(true)called at the right completion points (remote answer for local negotiations, local description for remote negotiations)- Default parameter preserves backward compatibility
Also applies to: 41-41, 51-51, 60-70, 105-105, 128-128
packages/media-signaling/src/lib/Call.ts (4)
192-194: LGTM! SDP exchange tracking flags properly initialized and scoped.The
sentLocalSdpandreceivedRemoteSdpflags correctly track the initial SDP exchange progress. They're appropriately scoped to theacceptedstate ingetClientState(), so they won't interfere with renegotiation flows.Also applies to: 233-234
423-444: LGTM! Granular state mapping aligns with WebRTC offer/answer flow.The logic correctly reflects the asymmetric nature of caller vs callee:
- Caller: generates offer first (
generating-local-sdp) → waits for answer (waiting-for-answer)- Callee: waits for offer first (
waiting-for-offer) → generates answer (generating-local-sdp)The fallback to
'waiting-for-offer'when no negotiation exists handles the edge case where the callee accepts before the caller sends an offer.
779-783: LGTM! Timeout reset on progress achieves the PR objective.Each client state transition within
'accepted'now starts a freshTIMEOUT_TO_PROGRESS_SIGNALINGtimer. Combined withupdateStateTimeouts()clearing old timers, this ensures valid calls with slow ICE gathering won't be prematurely dropped.
889-902: LGTM! Refactored SDP processing with proper early return.The switch statement cleanly handles offer/answer types, and the early
returnin the default case correctly prevents settingreceivedRemoteSdp = truefor unsupported SDP types.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/media-signaling/src/lib/services/webrtc/Negotiation.ts">
<violation number="1" location="packages/media-signaling/src/lib/services/webrtc/Negotiation.ts:179">
P1: Missing `throw err` after `fail()` call. Unlike `createEarlyAnswer` and `getPeerLocalDescription`, this method swallows the exception, causing callers to continue executing after a failure. This can lead to incorrect state (e.g., `end(true)` being called after failure in `setRemoteAnswer`).</violation>
<violation number="2" location="packages/media-signaling/src/lib/services/webrtc/Negotiation.ts:199">
P1: Missing `throw err` after `fail()` call. Unlike `createEarlyAnswer` and `getPeerLocalDescription`, this method swallows the exception, causing callers to continue executing after a failure. The subsequent `assertNegotiationIsActive()` won't detect this failure since it only checks `_ended`, not `_failed`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Proposed changes (including videos or screenshots)
When the callee client accepts a call, we were starting a 10s timeout waiting for the call to become active. This is usually more than enough because the process tends to be much faster than that and this timeout is intended only to cover the time that the client takes to generate the webrtc answer (including ICE Gathering). However, the user may accept the call before the caller client has sent the initial webrtc offer, in which case it needs to wait for the offer before it can start working in the answer. If there are multiple ICE servers delaying the ICE gathering process, this could cause the total amount of time between accepting the call and connecting to it to go over 10s, which would then drop the call.
This PR makes it so that the 10s timeout is reset whenever there's progress in the negotiation.
It also splits the
timeouthangup reason into 5 different values in order to specify which specific part of the process timed out.Issue(s)
VGA-114
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.